Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add testing data to the package distributions #421

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Add testing data to the package distributions #421

merged 1 commit into from
Jun 7, 2024

Conversation

avalentino
Copy link
Contributor

The test code pooch/tests is installed but he data in pooch/tests/data are not.
This makes it impossible to run tests on the installed package.

leouieda
leouieda approved these changes Jun 7, 2024
View reviewed changes
Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@leouieda leouieda changed the title Install package data Add tresting data to the distributions Jun 7, 2024
@leouieda leouieda changed the title Add tresting data to the distributions Add tresting data to the package distributions Jun 7, 2024
@leouieda leouieda changed the title Add tresting data to the package distributions Add testing data to the package distributions Jun 7, 2024
@leouieda leouieda merged commit 5824e57 into fatiando:main Jun 7, 2024
18 checks passed
@avalentino avalentino deleted the bugfix/package-data branch June 7, 2024 16:42
@avalentino
Copy link
Contributor Author

Good catch!

Thanks

@penguinpee penguinpee mentioned this pull request Jun 8, 2024
Closed
@kloczek
Copy link

kloczek commented Jun 25, 2024

Why at all test suite is distributed win .whl at all? 🤔
Better would be move test suite with its data to tests/ and distribute only in sdist.

@avalentino
Copy link
Contributor Author

@kloczek to me it makes totally sense to distribute test s and test data in sdist.
What could be an option, IMHO, is to not install them (i.e. do not include them in the .whl).

@kloczek
Copy link

kloczek commented Jun 25, 2024

test suite should not be distributed as part of the installable resources.
Currently it is included

Here is pep517 build output: 
+ /usr/bin/python3 -sBm build -w --no-isolation
* Getting build dependencies for wheel...
running egg_info
creating pooch.egg-info
writing pooch.egg-info/PKG-INFO
writing dependency_links to pooch.egg-info/dependency_links.txt
writing requirements to pooch.egg-info/requires.txt
writing top-level names to pooch.egg-info/top_level.txt
writing manifest file 'pooch.egg-info/SOURCES.txt'
reading manifest file 'pooch.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
no previously-included directories found matching '.github'
no previously-included directories found matching 'data'
no previously-included directories found matching 'env'
no previously-included directories found matching 'paper'
no previously-included directories found matching 'tools'
warning: no previously-included files found matching '.*.yml'
warning: no previously-included files found matching '.*rc'
warning: no previously-included files found matching 'Makefile'
warning: no previously-included files found matching '.gitignore'
warning: no previously-included files found matching '.gitattributes'
warning: no previously-included files found matching 'environment.yml'
warning: no files found matching 'pooch/tests/data'
adding license file 'LICENSE.txt'
adding license file 'AUTHORS.md'
writing manifest file 'pooch.egg-info/SOURCES.txt'
* Building wheel...
running bdist_wheel
running build
running build_py
creating build
creating build/lib
creating build/lib/doc
copying doc/conf.py -> build/lib/doc
creating build/lib/pooch
copying pooch/__init__.py -> build/lib/pooch
copying pooch/downloaders.py -> build/lib/pooch
copying pooch/hashes.py -> build/lib/pooch
copying pooch/processors.py -> build/lib/pooch
copying pooch/utils.py -> build/lib/pooch
copying pooch/core.py -> build/lib/pooch
copying pooch/_version.py -> build/lib/pooch
creating build/lib/pooch/tests
copying pooch/tests/__init__.py -> build/lib/pooch/tests
copying pooch/tests/test_hashes.py -> build/lib/pooch/tests
copying pooch/tests/test_integration.py -> build/lib/pooch/tests
copying pooch/tests/test_processors.py -> build/lib/pooch/tests
copying pooch/tests/test_utils.py -> build/lib/pooch/tests
copying pooch/tests/test_version.py -> build/lib/pooch/tests
copying pooch/tests/utils.py -> build/lib/pooch/tests
copying pooch/tests/test_core.py -> build/lib/pooch/tests
copying pooch/tests/test_downloaders.py -> build/lib/pooch/tests
running egg_info
writing pooch.egg-info/PKG-INFO
writing dependency_links to pooch.egg-info/dependency_links.txt
writing requirements to pooch.egg-info/requires.txt
writing top-level names to pooch.egg-info/top_level.txt
reading manifest file 'pooch.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
no previously-included directories found matching '.github'
no previously-included directories found matching 'data'
no previously-included directories found matching 'env'
no previously-included directories found matching 'paper'
no previously-included directories found matching 'tools'
warning: no previously-included files found matching '.*.yml'
warning: no previously-included files found matching '.*rc'
warning: no previously-included files found matching 'Makefile'
warning: no previously-included files found matching '.gitignore'
warning: no previously-included files found matching '.gitattributes'
warning: no previously-included files found matching 'environment.yml'
warning: no files found matching 'pooch/tests/data'
adding license file 'LICENSE.txt'
adding license file 'AUTHORS.md'
writing manifest file 'pooch.egg-info/SOURCES.txt'
installing to build/bdist.linux-x86_64/wheel
running install
running install_lib
creating build/bdist.linux-x86_64
creating build/bdist.linux-x86_64/wheel
creating build/bdist.linux-x86_64/wheel/doc
copying build/lib/doc/conf.py -> build/bdist.linux-x86_64/wheel/doc
creating build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/__init__.py -> build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/downloaders.py -> build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/hashes.py -> build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/processors.py -> build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/utils.py -> build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/core.py -> build/bdist.linux-x86_64/wheel/pooch
copying build/lib/pooch/_version.py -> build/bdist.linux-x86_64/wheel/pooch
creating build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/__init__.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_hashes.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_integration.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_processors.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_utils.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_version.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/utils.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_core.py -> build/bdist.linux-x86_64/wheel/pooch/tests
copying build/lib/pooch/tests/test_downloaders.py -> build/bdist.linux-x86_64/wheel/pooch/tests
running install_egg_info
Copying pooch.egg-info to build/bdist.linux-x86_64/wheel/pooch-1.8.2-py3.10.egg-info
running install_scripts
creating build/bdist.linux-x86_64/wheel/pooch-1.8.2.dist-info/WHEEL
creating '/home/tkloczko/rpmbuild/BUILD/pooch-1.8.2/dist/.tmp-9q64we78/pooch-1.8.2-py3-none-any.whl' and adding 'build/bdist.linux-x86_64/wheel' to it
adding 'doc/conf.py'
adding 'pooch/__init__.py'
adding 'pooch/_version.py'
adding 'pooch/core.py'
adding 'pooch/downloaders.py'
adding 'pooch/hashes.py'
adding 'pooch/processors.py'
adding 'pooch/utils.py'
adding 'pooch/tests/__init__.py'    <<<<<<=== FROM HERE
adding 'pooch/tests/test_core.py'
adding 'pooch/tests/test_downloaders.py'
adding 'pooch/tests/test_hashes.py'
adding 'pooch/tests/test_integration.py'
adding 'pooch/tests/test_processors.py'
adding 'pooch/tests/test_utils.py'
adding 'pooch/tests/test_version.py'
adding 'pooch/tests/utils.py'
adding 'pooch-1.8.2.dist-info/AUTHORS.md'
adding 'pooch-1.8.2.dist-info/LICENSE.txt'
adding 'pooch-1.8.2.dist-info/METADATA'
adding 'pooch-1.8.2.dist-info/WHEEL'
adding 'pooch-1.8.2.dist-info/top_level.txt'
adding 'pooch-1.8.2.dist-info/RECORD'
removing build/bdist.linux-x86_64/wheel
Successfully built pooch-1.8.2-py3-none-any.whl

Looks like easiest way to do that would be just move pooch/tests/ to tests/
If test suite is not using relative imports .. if it uses then those relative imports needs to be dropped (using relative imports is always is nothing more than asking for troubles ..).

@kloczek
Copy link

kloczek commented Jun 25, 2024

Just checked source code and relative imports are in use 😞
In first file pooch/tests/utils.py I found:

from .. import __version__ as full_version
from ..utils import check_version, get_logger

@santisoler
Copy link
Member

Distributing tests with the source code was a design decision that dates back to the origin of the package. This was intended to allow users to run tests on the installed version of the package without the need to clone the repository. We saw value on it back in the days. We even included instructions on how to run the tests after installation (see the old docs: https://www.fatiando.org/pooch/v1.0.0/install.html#testing-your-install). The relative imports were also intended: since tests were part of the code base, it made sense to use relative imports as we would do in any submodule of the package.

Nonetheless, we've been discussing about removing the test suite from the package since users rarely test the code after installation, and we would like to reduce the size of the packages that ppl download, specially since Pooch is being widely used by many packages in the community. Check out fatiando/community#154, #423, #416 and #427 for more details.

I appreciate your suggestions on this matter. Although, I don't think these design decisions were bad in nature. We wanted to provide our users with a feature (run tests after installation), which now we are reconsidering in favor of reducing installation size. There's no PEP rule against shipping tests along with code (in fact many heavily used scientific Python packages ship tests. See Numpy, matplotlib, scikit-learn as examples). In fact, including tests in the code of the package is one of the test layouts recommended by pytest. Therefore, the decisions to include the tests folder within the sources or outside of it, or to ship them or not are mostly tied to the preferences and needs of the maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leouieda leouieda leouieda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@avalentino @kloczek @santisoler @leouieda